-
Notifications
You must be signed in to change notification settings - Fork 153
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement UI for creating new playlists #234
Conversation
a238a01
to
77c08d0
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good. Thanks for the PR and taking time to implement this feature. A few suggestions.
The desc field looks to be broken upstream in rspotify - the new playlist does not seem to have this informatino attached. Both of these limitations are also observed in the CLI implementation
Can you elaborate by "the new playlist does not seem to have this information attached"? desc
seems to work fine for me. I can see the description of new playlist created by spotify_player
in the official Spotify app.
once there are more instances of the pop-up with input prompt (similar to both Search and PlaylistCreate here), it should be abstracted.
nice to have for sure but no need to implement one in this PR.
desc, | ||
} => { | ||
let user_id = state.data.read().user_data.user.to_owned().unwrap().id; | ||
let _ = self |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can have a info
log here saying a new playlist with ... id was successfully created.
let _ = self | ||
.create_new_playlist( | ||
state, | ||
user_id.clone(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
redundant clone?
Playlist { | ||
id: resp.id.to_owned(), | ||
collaborative: collab, | ||
name: playlist_name.to_string(), | ||
owner: ("".into(), user_id), | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should use resp.into()
instead. state::Playlist
does implement the From<rspotify_model::FullPlaylist>
trait.
@@ -204,6 +206,8 @@ impl Command { | |||
Self::ReverseTrackOrder => "reverse the order of the track table (if any)", | |||
Self::MovePlaylistItemUp => "move playlist item up one position", | |||
Self::MovePlaylistItemDown => "move playlist item down one position", | |||
|
|||
Self::CreatePlaylist => "create new playlist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self::CreatePlaylist => "create new playlist", | |
Self::CreatePlaylist => "create a new playlist", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you update https://github.com/aome510/spotify-player#commands as well?
.split(chunks[1]); | ||
|
||
let name_input = construct_and_render_block( | ||
"Enter Name for New Playlist:", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Enter Name for New Playlist:", | |
"Enter Name for New Playlist", |
if !name.is_empty() { | ||
name.pop().unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !name.is_empty() { | |
name.pop().unwrap(); | |
} | |
name.pop(); |
no need to check is_empty
then unwrap
if !desc.is_empty() { | ||
desc.pop().unwrap(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !desc.is_empty() { | |
desc.pop().unwrap(); | |
} | |
desc.pop(); |
} | ||
return Ok(true); | ||
} | ||
crossterm::event::KeyCode::Tab => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crossterm::event::KeyCode::Tab => { | |
crossterm::event::KeyCode::Tab | crossterm::event::KeyCode::BackTab => { |
can also handle shift+Tab
let page_type = state.ui.lock().current_page().page_type(); | ||
match page_type { | ||
PageType::Library => page::handle_key_sequence_for_library_page(key_sequence, state), | ||
PageType::Search => { | ||
page::handle_key_sequence_for_search_page(key_sequence, client_pub, state) | ||
} | ||
PageType::Context => { | ||
page::handle_key_sequence_for_context_page(key_sequence, client_pub, state) | ||
} | ||
PageType::Browse => { | ||
page::handle_key_sequence_for_browse_page(key_sequence, client_pub, state) | ||
} | ||
#[cfg(feature = "lyric-finder")] | ||
PageType::Lyric => { | ||
page::handle_key_sequence_for_lyric_page(key_sequence, client_pub, state) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to do this for the create playlist popup. Search popup is special because the focus is not placed on it when the popup is created. It's expected to use along with the focused window for interactive searching.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just return Ok(false)
in this case.
Command::CreatePlaylist => { | ||
ui.popup = Some(PopupState::PlaylistCreate { | ||
name: "".into(), | ||
desc: "".into(), | ||
current_field: PlaylistCreateCurrentField::Name, | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't handle the create playlist command here.
I just tested this again today, and the description is now transmitted properly. On the day I was implementing this feature, that was not the case - neither through the popup or the CLI. Weird. |
Thanks for the great suggestions, will work through them when time permits. |
This feature looks great ✨ |
I'm still waiting for updates from @nuugen. I can take over if time permits. |
Hi, I noticed that this PR is stuck for some time now. May I finish this implementation as this feature would be really useful? |
Hi, sorry all for the hold-up. @m-torhan, please, go ahead with finishing this feature. Thanks! |
Close in favour of #365 |
This attempts to implement a rudimentary UI flow for creating a new playlist.
Resolves #87.
Usage:
While focused on the playlist window, pressing
N
will show a pop-up prompting user to enter the name and the description of the new playlist to be created. PressingTab
here will switch focus between the two fields.Limitation:
For simplicity, the
public
andcollab
statuses will be omitted.TheThis seems to be a misobservation.desc
field looks to be broken upstream inrspotify
- the new playlist does not seem to have this informatino attached. Both of these limitations are also observed in the CLI implementation (#222).I'm also relatively new to Rust, so I mainly focused on submitting a purely functioning implementation, without much regard to the DRYness of code - once there are more instances of the pop-up with input prompt (similar to both
Search
andPlaylistCreate
here), it should be abstracted.Suggestions and critics are of course more than welcome and would help me a lot getting started in my Rust journey.